-
Notifications
You must be signed in to change notification settings - Fork 824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(interface-types) Implement the record
instructions
#1331
Merged
bors
merged 44 commits into
wasmerio:master
from
Hywan:feat-interface-types-record-instructions
Apr 6, 2020
Merged
feat(interface-types) Implement the record
instructions
#1331
bors
merged 44 commits into
wasmerio:master
from
Hywan:feat-interface-types-record-instructions
Apr 6, 2020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hywan
added
🎉 enhancement
New feature!
🧪 tests
I love tests
📚 documentation
Do you like to read?
📦 lib-interface-types
labels
Mar 24, 2020
This patch updates the `Type` type to be an enum with 2 variants: `Function` and `Record`, resp. to represent: 1. `(@interface type (func (param i32 i32) (result string)))` 2. `(@interface type (record string i32))` This patch updates the binary encoder and decoder, along with the WAT encoder and decoder.
Hywan
force-pushed
the
feat-interface-types-record-instructions
branch
from
March 26, 2020 09:55
68f248f
to
bbb4f1f
Compare
…`Type`. The `Type::Record` variant now is defined by `RecordType`. In addition, `InterfaceType` has a new variant: `Record`, that is also defined by `RecordType`. Encoders and decoders are updated to consider `RecordType`, which removes code duplication and simplify code.
Hywan
force-pushed
the
feat-interface-types-record-instructions
branch
from
March 26, 2020 12:35
61b4a42
to
bd9226e
Compare
…o Rust values. WIT values are native Rust values. But records are represented as a vector of WIT values. In order to provide a super neat API to the user, Serde is used to deserialize this vector of WIT values to a large variety of Rust values.
Hywan
force-pushed
the
feat-interface-types-record-instructions
branch
from
March 31, 2020 10:34
1c0f894
to
02b7e21
Compare
…s::FlattenInterfaceValueIterator`.
`seq`, `map` and `tuple` for instance are not supported officially. It was fun to play with it at the beginning, but it is time to remove it to match the `Serializer` implementation.
… into `serde/ser.rs` and `serde/de.rs`. I like smaller files.
MarkMcCaskey
approved these changes
Apr 2, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
…ctor. `Vec1` is used by `RecordType` to ensure that a record have at least 1 field. Then an `InterfaceValue::Record` is ensured to get at least one value with the type-checking pass.
`into_iter` owned the item, so no need to clone when moving them in the stack.
Using `InterfaceValue::Record` explicitely doesn't change anything since values are flatten, but it's better for a usual reading to avoid confusion.
bors r+ |
Build succeeded
|
Hywan
added a commit
to Hywan/wasmer
that referenced
this pull request
Apr 6, 2020
The opened questions mentionned in the description of this PR are discussed here, WebAssembly/interface-types#108. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR implements the
record
WIT type, along with therecord.lift
andrecord.lower
instructions.With my current understanding of the draft/specification, here is how it works. Let's say we want to represent a Rust struct like the following:
First declare a WIT type, such as:
The
record
type is supported by the binary encoder, the WAT encoder, the binary decoder, and the WAT decoder. A newTypeKind
node has been introduced in the AST to differentiate a function type ((@interface type (func (param …) (result …)))
) of a record type (see above).Second, the
record.lower
transforms a host value (here Rust value,S
) into a WIT value. In our implementation, a record value is defined as:Multiple mechanisms are used to type check a record value based on a record type. The code of the
record.lower
is pretty straightforward.Because transforming a host value into a WIT value isn't obvious, a
Serializer
has been implemented, based onserde
. This feature is behind theserde
flag, which is turned on by default.Serde is only used to cross the host/WIT boundary, but it's not used to represent the value in memory or anything. It's only a shortcut to transform a host value into a WIT value, and vice versa.
Use the following code to transform
S
into a WIT value:Third, the
record.lift
instruction does the opposite ofrecord.lower
: It transforms WIT values into a host value. To also facilitate the user experience, this PR contains aDeserializer
implementation, still based onserde
, with thefrom_interface_values
function. It looks like this:With the
Serializer
andDeserializer
, it's super easy for the user to send or receive values from WIT.The
record.lift
andrecord.lower
instructions are kind of basic. Therecord.lift
instruction has a little trick to reduce vector allocations, but there is a documentation for that.Opened questions
Edit: Discussed here, WebAssembly/interface-types#108
Records of dimension 1 do not raise any issue. With
record.lift
, all values on the stack (the WIT interpreter stack) are popped, and are used as record field values. Something like:generates
But it's not clear what happens with record of dimension > 1, for instance for a type like
record (field i32) (record (field i32) (field i32)) (field string)
, it is assumed (in this PR) that the stack must be like this:to generate:
If we want the stack to contain an intermediate record, we should have something like this:
But it would imply that
record_type_1
is defined asrecord (field i32) (record (type record_type_2)) (field i32)
.A sub-record defined by another record type isn't support, as it is not specified in the draft. I believe my assumption is fine enough for a first implementation of records in WIT.
To do
(@interface type (record string i32))
):record.lift
instructionrecord.lower
instructionrecord
s)record
s)